ffi: add new methods and error codes by ShogunPanda · Pull Request #62762 · nodejs/node

ShogunPanda · 2026-04-15T18:28:25Z

Added null support in DynamicLibrary loading on non-Windows (dlopen(NULL) semantics), plus docs clarifying null is not supported on Windows.
Added a new FFI benchmark benchmark/ffi/getpid.js using ffi.dlopen(null, ...) + uv_os_getpid. Thanks to @bengl for Foreign Function Interface (FFI) implementation #46905
Documented new FFI error codes in doc/api/errors.md as asked from @jasnell in lib,src,test,doc: add node:ffi module #62072 (comment)
Code documented behavior of why JS callback may abort the process. This address what requested by @jasnell in lib,src,test,doc: add node:ffi module #62072 (comment).

As promised to @justjake, also the following followups requested in #62072 (comment):

Added ffi.exportArrayBuffer(...) and ffi.exportArrayBufferView(...).
Added raw pointer helper ffi.getRawPointer(source) (Buffer | ArrayBuffer | ArrayBufferView), with strong “unsafe/dangerous” docs and tests.
Moved byte-export copying to native code (exportBytes) to avoid extra JS Buffer wrapper allocations.

nodejs-github-bot · 2026-04-15T18:28:31Z

Review requested:

@nodejs/config
@nodejs/gyp
@nodejs/performance

justjake · 2026-04-15T18:32:06Z

Great! Thank you. It looks like I won't need my own unsafe-pointer once node:ffi gets stabilized 🎉 😄

codecov · 2026-04-15T19:52:19Z

Codecov Report

❌ Patch coverage is 60.71429% with 88 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.68%. Comparing base (d7ab027) to head (d33227e).
⚠️ Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ffi/data.cc	49.48%	33 Missing and 16 partials ⚠️
src/node_ffi.cc	46.80%	24 Missing and 1 partial ⚠️
src/ffi/types.cc	57.57%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62762      +/-   ##
==========================================
- Coverage   89.69%   89.68%   -0.01%     
==========================================
  Files         706      706              
  Lines      218143   218400     +257     
  Branches    41730    41805      +75     
==========================================
+ Hits       195655   195864     +209     
- Misses      14401    14419      +18     
- Partials     8087     8117      +30

Files with missing lines	Coverage Δ
lib/ffi.js	`96.56% <100.00%> (+0.71%)`	⬆️
src/node_errors.h	`86.48% <ø> (ø)`
src/node_ffi.h	`72.72% <ø> (ø)`
src/ffi/types.cc	`59.61% <57.57%> (-0.06%)`	⬇️
src/node_ffi.cc	`69.04% <46.80%> (-0.04%)`	⬇️
src/ffi/data.cc	`73.28% <49.48%> (-5.50%)`	⬇️

... and 41 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

meixg · 2026-04-16T07:41:30Z

LGTM
One small API thought: would it be better to expose this as something like getRawReference() instead of getRawPointer(), returning both the pointer and byteLength? For byte sources, the pointer is usually
meant to be used together with its length.

ShogunPanda · 2026-04-16T09:51:45Z

@meixg I don't think it is acually necessary. When using the function you already own the source (getRawPointer(buffer)) so you can call the byteLength directly, isn't it?

Signed-off-by: Paolo Insogna <paolo@cowtech.it>

addaleax · 2026-04-17T11:53:53Z

+  if (Buffer::HasInstance(args[0])) {
+    ptr = reinterpret_cast<uintptr_t>(Buffer::Data(args[0]));
+  } else if (args[0]->IsArrayBuffer()) {


This is already covered by the IsArrayBufferView branch below

Suggested change

if (Buffer::HasInstance(args[0])) {

ptr = reinterpret_cast<uintptr_t>(Buffer::Data(args[0]));

} else if (args[0]->IsArrayBuffer()) {

if (args[0]->IsArrayBuffer()) {

addaleax · 2026-04-17T11:54:22Z

      std::memchr(*value, '\0', value.length()) != nullptr) {
-    env->ThrowTypeError((label + " must not contain null bytes").c_str());
+    THROW_ERR_INVALID_ARG_VALUE(
+        env, (label + " must not contain null bytes").c_str());


Suggested change

env, (label + " must not contain null bytes").c_str());

env, "%s must not contain null bytes", label);

Ideally, this would be applied to all other formatting operations too

addaleax · 2026-04-17T11:54:28Z

                      " must have either 'returns', 'return' or 'result' "
                      "property";
-    env->ThrowTypeError(msg.c_str());
+    THROW_ERR_INVALID_ARG_VALUE(env, msg.c_str());


Suggested change

THROW_ERR_INVALID_ARG_VALUE(env, msg.c_str());

THROW_ERR_INVALID_ARG_VALUE(env, msg);

addaleax · 2026-04-17T11:54:35Z

                      " must have either 'parameters' or 'arguments' "
                      "property";
-    env->ThrowTypeError(msg.c_str());
+    THROW_ERR_INVALID_ARG_VALUE(env, msg.c_str());


Suggested change

THROW_ERR_INVALID_ARG_VALUE(env, msg.c_str());

THROW_ERR_INVALID_ARG_VALUE(env, msg);

addaleax · 2026-04-17T11:54:41Z

      std::string msg =
          "Return value type of function " + name + " must be a string";
-      env->ThrowTypeError(msg.c_str());
+      THROW_ERR_INVALID_ARG_VALUE(env, msg.c_str());


Suggested change

THROW_ERR_INVALID_ARG_VALUE(env, msg.c_str());

THROW_ERR_INVALID_ARG_VALUE(env, msg);

addaleax · 2026-04-17T11:58:37Z

          ("Argument " + std::to_string(index) +
           " must be a buffer, an ArrayBuffer, a string, or a bigint")
              .c_str());


Suggested change

("Argument " + std::to_string(index) +

" must be a buffer, an ArrayBuffer, a string, or a bigint")

.c_str());

"Argument %s"

" must be a buffer, an ArrayBuffer, a string, or a bigint", index);

addaleax · 2026-04-17T11:58:56Z

+                                    ("Argument " + std::to_string(index) +
+                                     " must be a non-negative pointer bigint")
+                                        .c_str());


Suggested change

("Argument " + std::to_string(index) +

" must be a non-negative pointer bigint")

.c_str());

"Argument %s"

" must be a non-negative pointer bigint", index);

addaleax · 2026-04-17T11:59:30Z

+            ("Invalid ArrayBuffer backing store for argument " +
+             std::to_string(index))
+                .c_str());


Suggested change

("Invalid ArrayBuffer backing store for argument " +

std::to_string(index))

.c_str());

"Invalid ArrayBuffer backing store for argument %s",

index);

addaleax · 2026-04-17T11:59:59Z

            ("Invalid ArrayBufferView backing store for argument " +
             std::to_string(index))
                .c_str());


Suggested change

("Invalid ArrayBufferView backing store for argument " +

std::to_string(index))

.c_str());

"Invalid ArrayBufferView backing store for argument %s", index);

addaleax · 2026-04-17T12:00:25Z

+  if (Buffer::HasInstance(args[0])) {
+    source_data = reinterpret_cast<uint8_t*>(Buffer::Data(args[0]));
+    source_len = Buffer::Length(args[0]);
+  } else if (args[0]->IsArrayBuffer()) {


ditto here

Suggested change

if (Buffer::HasInstance(args[0])) {

source_data = reinterpret_cast<uint8_t*>(Buffer::Data(args[0]));

source_len = Buffer::Length(args[0]);

} else if (args[0]->IsArrayBuffer()) {

if (args[0]->IsArrayBuffer()) {

And more broadly than that, you can probably just implement this a lot more easily by using ArrayBufferViewContents

addaleax · 2026-04-17T15:53:53Z

+      return;
+    }
+    library_path = *filename;
+    lib->path_ = std::string(*filename);


Suggested change

lib->path_ = std::string(*filename);

lib->path_ = filename.ToString();

addaleax · 2026-04-17T15:54:13Z

+    if (ThrowIfContainsNullBytes(env, filename, "Library path")) {
+      return;
+    }
+    library_path = *filename;


This is a use-after-free bug – *filename goes out of scope outside of this conditional

addaleax · 2026-04-17T15:55:09Z

-    env->ThrowRangeError(
-        (std::string("The ") + label + " is too large").c_str());
+    THROW_ERR_OUT_OF_RANGE(
+        env, (std::string("The ") + label + " is too large").c_str());


Suggested change

env, (std::string("The ") + label + " is too large").c_str());

"The %s is too large", label);

addaleax · 2026-04-17T15:55:46Z

        (std::string("The ") + label + " exceeds the platform pointer range")
            .c_str());


Suggested change

(std::string("The ") + label + " exceeds the platform pointer range")

.c_str());

"The %s exceeds the platform pointer range", label);

addaleax · 2026-04-17T15:57:16Z

+  if (Buffer::HasInstance(args[0])) {
+    source_data = reinterpret_cast<uint8_t*>(Buffer::Data(args[0]));
+    source_len = Buffer::Length(args[0]);
+  } else if (args[0]->IsArrayBuffer()) {


And more broadly than that, you can probably just implement this a lot more easily by using ArrayBufferViewContents

Signed-off-by: Paolo Insogna <paolo@cowtech.it>

ShogunPanda · 2026-04-18T11:55:29Z

@addaleax I think I addressed all your suggestions. Do you mind checking it again?

nodejs-github-bot · 2026-04-18T12:31:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/72754/

nodejs-github-bot · 2026-04-18T14:19:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/72757/

mcollina

lgtm

addaleax · 2026-04-18T21:39:34Z

+
+<a id="ERR_FFI_SYSCALL_FAILED"></a>
+
+### `ERR_FFI_SYSCALL_FAILED`


I'd say this is a bad name unless it actually has anything to do with syscalls, which is a pretty specific pre-existing term

Indeed, I made other comments assuming that FFI_SYSCALL, though poorly named, should be taken to be something more like FFI_CALL, but instead it seems like it's being used as a more generic error.

I couldn't find another example in errors.md, but maybe ERR_FFI_ERROR is generic enough? Otherwise, being specific for each of the error cases might be in order.

addaleax · 2026-04-18T21:44:48Z

+      THROW_ERR_INVALID_ARG_VALUE(env, "Invalid ArrayBufferView backing store");
+      return;
+    }
+    source_data = const_cast<uint8_t*>(view.data());


This const_cast should not be necessary if source_data is a const uint8_t* (which it should be anyway)

addaleax · 2026-04-18T21:48:03Z

+    return;
+  }
+
+  uintptr_t ptr = 0;


This code makes the assumption that a BackingStore will never move even if no references are held by embedders; I am not sure if this is something that V8 guarantees, or something that we should rely on.

We can add this as experimental but we really, really need to evaluate whether this is an acceptable model that is guaranteed to be usable in the long term or not

bengl · 2026-04-19T02:44:52Z

    if (uv_dlsym(&lib_, name.c_str(), &ptr) != 0) {
      std::string msg = std::string("dlsym failed: ") + uv_dlerror(&lib_);
-      env->ThrowError(msg.c_str());
+      THROW_ERR_FFI_SYSCALL_FAILED(env, msg.c_str());


This is not an FFI call that's failing, but the resolution of a symbol.

bengl · 2026-04-19T02:47:54Z

      }

-      env->ThrowError(msg);
+      THROW_ERR_FFI_SYSCALL_FAILED(env, msg);


This is also not an FFI call that's failing.

ShogunPanda requested review from bengl and jasnell April 15, 2026 18:28

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 15, 2026

ShogunPanda mentioned this pull request Apr 15, 2026

lib,src,test,doc: add node:ffi module #62072

Closed

justjake mentioned this pull request Apr 15, 2026

Node.js support anomalyco/opentui#939

Open

meixg approved these changes Apr 16, 2026

View reviewed changes

ShogunPanda changed the title ~~ffi: added new methods and error codes~~ ffi: add new methods and error codes Apr 16, 2026

ShogunPanda force-pushed the ffi-followup branch from 927da3b to 3e3b1da Compare April 16, 2026 15:15

lib: add new methods and error codes

1fee1dd

Signed-off-by: Paolo Insogna <paolo@cowtech.it>

ShogunPanda force-pushed the ffi-followup branch from 3e3b1da to 1fee1dd Compare April 16, 2026 15:17

addaleax reviewed Apr 17, 2026

View reviewed changes

fixup

d33227e

Signed-off-by: Paolo Insogna <paolo@cowtech.it>

mcollina approved these changes Apr 18, 2026

View reviewed changes

addaleax approved these changes Apr 18, 2026

View reviewed changes

bengl reviewed Apr 19, 2026

View reviewed changes

	env, (label + " must not contain null bytes").c_str());
	env, "%s must not contain null bytes", label);

	THROW_ERR_INVALID_ARG_VALUE(env, msg.c_str());
	THROW_ERR_INVALID_ARG_VALUE(env, msg);

	lib->path_ = std::string(*filename);
	lib->path_ = filename.ToString();

	env, (std::string("The ") + label + " is too large").c_str());
	"The %s is too large", label);

		(std::string("The ") + label + " exceeds the platform pointer range")
		.c_str());

	(std::string("The ") + label + " exceeds the platform pointer range")
	.c_str());
	"The %s exceeds the platform pointer range", label);


		<a id="ERR_FFI_SYSCALL_FAILED"></a>

		### `ERR_FFI_SYSCALL_FAILED`

Uh oh!

Conversation

ShogunPanda commented Apr 15, 2026

Uh oh!

nodejs-github-bot commented Apr 15, 2026

Uh oh!

justjake commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

meixg commented Apr 16, 2026

Uh oh!

ShogunPanda commented Apr 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShogunPanda commented Apr 18, 2026

Uh oh!

nodejs-github-bot commented Apr 18, 2026

Uh oh!

nodejs-github-bot commented Apr 18, 2026

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

justjake commented Apr 15, 2026 •

edited

Loading

codecov bot commented Apr 15, 2026 •

edited

Loading